Skip to content
This repository was archived by the owner on Aug 1, 2021. It is now read-only.

Comments

add links for depedencies, extra licenses, and output as file#20

Open
mayel wants to merge 4 commits intounnawut:masterfrom
bonfire-networks:pr
Open

add links for depedencies, extra licenses, and output as file#20
mayel wants to merge 4 commits intounnawut:masterfrom
bonfire-networks:pr

Conversation

@mayel
Copy link

@mayel mayel commented Feb 16, 2021

Hope others find this useful... Also tweaked the logic so as to properly recognise at least all the deps in my project :)

@coveralls
Copy link

coveralls commented Feb 16, 2021

Pull Request Test Coverage Report for Build 60

  • 30 of 38 (78.95%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 65.052%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/licensir/guesser.ex 2 3 66.67%
lib/mix/tasks/licenses.ex 18 21 85.71%
lib/licensir/scanner.ex 4 8 50.0%
Files with Coverage Reduction New Missed Lines %
lib/csv/encoding/encode.ex 1 75.0%
Totals Coverage Status
Change from base Build 54: 0.3%
Covered Lines: 188
Relevant Lines: 289

💛 - Coveralls

Copy link
Owner

@unnawut unnawut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I made a few comments. Feel free to counter me on any of them. :)

Map.put(license, :license, conclusion)
end

defp guess(file, ""), do: guess(file, nil)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, there shouldn't be any case where the 2nd argument is an empty string. So I think if it falls into this case we should fix the root cause than handling it here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a case of a dependency with no license which results in an empty license column in the final rather than "Undefined" if this line is present.

end

defp render(rows, opts) do
if Keyword.get(opts, :only_license),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the short hand if: ..., do: ..., else: ... are discouraged for multi-line conditionals per the popular code style.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are single lines though

@mayel
Copy link
Author

mayel commented Feb 22, 2021

How does it look now?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants